Java Assignment3 upload by SeongminLee#39
Java Assignment3 upload by SeongminLee#39MebukiYamashita wants to merge 1 commit intoFastCampusKDTBackend:mainfrom
Conversation
MinChul-Son
left a comment
MinChul-Son
left a comment
There was a problem hiding this comment.
안녕하세요 성민님~
과제 진행하신다고 고생많으셨습니다.
깊은 복사의 개념에 대해 확실히 집고가셨으면 좋겠습니다. 메모리 주소값과 연관있는 굉장히 중요한 지식이에요!
Arrays.copyOf()를 사용해준 것을 보니 어느정도 알고 계신 것 같은데요. 배열안에 참조형이 존재하므로 한번 더 작업이 필요할 것 같네요~
코드를 작성하실 때 코드 스타일에 대해서도 조금 신경 써주셨으면 좋겠습니다. 인텔리제이의 자동 정렬 기능을 사용해보세요~
메서드, 변수들의 위치, 줄바꿈 등등
깃 커밋의 단위를 좀 더 잘개 쪼개면 좋을 것 같아요!
ex)
- feat: User 클래스 생성 및 필드 작성
- feat: 생성자 생성
- feat: 싱글톤 패턴 구현
- feat: 회원 조회 메서드 구현
- refactor: 회원 조회 메서드 로직 분리
요렇게요!!
커밋 내역만으로 어떤 작업을 했는지 알 수 있다면 추후 협업 과정에서 큰 도움을 받을 수 있을 거에요~!
다음 과제에서는 요구사항을 좀 더 꼼꼼히 읽어보셨으면 좋겠습니다!
그리고 pr을 올리실 때 본인만의 템플릿을 가지고 계시면 좋을 것 같습니다.
ex)
- 내가 중점적으로 생각한 부분
- 작업 내용
- 궁금한 점
- 함께 이야기하고 싶은 부분
| private String userPassword; | ||
| private String userPhoneNumber; | ||
| private String userEmail; | ||
| private String userBirthDate; |
There was a problem hiding this comment.
생년월일을 표현하는 변수가 String은 조금 어색한 것 같습니다. 적절한 것이 없을까요?
| private Electronic[] electronicDevices; | ||
| private LocalDateTime registerTime; | ||
|
|
||
| public User() { |
There was a problem hiding this comment.
해당 생성자가 내부에서만 사용된다면 private으로 지정해도 괜찮을 것 같네요~
| this.userBirthDate = userBirthDate; | ||
| } | ||
|
|
||
| public Electronic[] getElectronicDevices() { |
There was a problem hiding this comment.
여기서 문제~!
이렇게 반환된 배열이 외부에서 변경된다면 원본에 영향이 있을까요? 없을까요?
|
|
||
| public class Electronic { | ||
|
|
||
| public enum Company { SAMSUNG, LG, APPLE } |
| this.modelName = modelName; | ||
| this.company = company; | ||
| this.authMethods = authMethods; | ||
| this.productNo = generateProductNo(); |
There was a problem hiding this comment.
사소하지만 멤버 변수 선언 순서와 생성자에서의 순서를 일치시켜주시면 좋을 것 같습니다.
|
|
||
| public class Users { | ||
| private static Users instance; | ||
| private User[] userList; |
There was a problem hiding this comment.
List라는 단어가 굳이 변수명에 포함될 필요는 없을 것 같아요. 심지어 List도 아닌 배열이네요😀
| private static Users instance; | ||
| private User[] userList; | ||
|
|
||
| private Users() { |
|
|
||
| public User findByUserId(String userId) { | ||
| if (userList == null) { | ||
| return null; |
There was a problem hiding this comment.
꼼꼼하게 널 체크를 해주는 습관은 좋습니다.
다만, null을 바로 반환하지말고 빈 객체, Optional 또는 적절한 예외를 반환하는게 어떨까요?
| if (userList == null) { | ||
| return null; | ||
| } | ||
| for (User userInfo : userList) { |
There was a problem hiding this comment.
동일한 동작을 Stream을 사용해서 수행할 수 있을 것 같습니다. 어떻게 하면 좋을까요?
| copiedUser.setUserPassword(user.getUserPassword()); | ||
| copiedUser.setUserBirthDate(user.getUserBirthDate()); | ||
| copiedUser.setUserPhoneNumber(user.getUserPhoneNumber()); | ||
| copiedUser.setElectronicDevices(Arrays.copyOf(user.getElectronicDevices(), user.getElectronicDevices().length)); |
There was a problem hiding this comment.
Cloneable 인터페이스와 clone() 메서드에 대해 추가적으로 학습해봐요.
현재 상황에서는 User 내부의 Electronic[]을 복사본과 원본이 공유하게 됩니다.
2 participants
과제 코드리뷰 감사합니다. 이번주까지는 바빠서 금요일부터 3일간 잘못된 부분이나 부족한 부분 읽겠습니다.
이번 코드도; 오후 한시부터 부랴부랴 써서 낸거라 제대로 된건지는 모르겠습니다...
추후에 다시 읽고 코드 수정해보겠습니다